Conversation
WalkthroughRefactors alt-token balance-slot handling by adding Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as state_transition / token_gas callers
participant TokenInfo as fees.TokenInfo
participant Transfer as TransferAltTokenHybrid
participant State as fees.TransferAltTokenByState
participant EVM as fees.TransferAltTokenByEVM
Note over Caller,TokenInfo: Caller passes tokenInfo to TransferAltTokenHybrid
Caller->>Transfer: TransferAltTokenHybrid(tokenInfo, from, to, amount, userBalanceBefore)
alt tokenInfo.HasSlot == true
Transfer->>State: fees.TransferAltTokenByState(tokenInfo.TokenAddress, tokenInfo.BalanceSlot, ...)
State-->>Transfer: result / err
else tokenInfo.HasSlot == false
Transfer->>EVM: fees.TransferAltTokenByEVM(tokenInfo.TokenAddress, ...)
EVM-->>Transfer: result / err
end
Transfer-->>Caller: returns error / success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/token_gas.go (1)
66-73: Update to useHasSlotfor consistency.The condition still uses the old pattern of comparing
info.BalanceSlot.Bytes()to the zero hash. For consistency withGetAltTokenBalanceHybrid(line 31) and the newHasSlotfield, refactor this to useinfo.HasSlot.Apply this diff:
- if !bytes.Equal(info.BalanceSlot.Bytes(), common.Hash{}.Bytes()) { - // balance slot exist + if info.HasSlot { balance, _, err = fees.GetAltTokenBalanceFromSlot(evm.StateDB, info.TokenAddress, user, info.BalanceSlot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/state_transition.go(2 hunks)core/token_gas.go(2 hunks)rollup/fees/token_info.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/state_transition.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
📚 Learning: 2025-11-10T06:56:24.937Z
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
Applied to files:
core/token_gas.go
🧬 Code graph analysis (2)
core/token_gas.go (3)
core/state_transition.go (1)
StateTransition(65-82)rollup/fees/token_info.go (1)
TokenInfo(29-36)rollup/fees/token_transfer.go (1)
TransferAltTokenByState(12-37)
rollup/fees/token_info.go (1)
common/types.go (1)
BigToHash(62-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
rollup/fees/token_info.go (3)
35-35: Good addition for clarity.The
HasSlotfield explicitly indicates whether a token uses a storage slot for balance tracking, improving code readability compared to checking ifBalanceSlotis the zero hash.
139-139: LGTM!The
HasSlotfield is correctly populated from the computedhasSlotvalue.
109-118: I need to search the codebase more broadly for wherebalanceSlotis encoded (set) rather than just decoded. Let me look for all file types and test files.Document the slot encoding convention with a clear comment explaining the -1 adjustment and link to contract implementation.
The code subtracts 1 from
balanceSlotValue(line 116), assuming the contract storesactualSlot + 1to distinguish slot 0 from "no slot" (zero hash). This encoding convention is not documented in the code or referenced in standard ERC20 patterns. Add an inline comment explaining this encoding scheme and reference the contract implementation that establishes this convention to ensure consistency.core/token_gas.go (3)
31-31: Good refactoring to useHasSlot.Using
!info.HasSlotis clearer and more explicit than checking if the balance slot equals the zero hash.
47-47: Excellent API simplification.Consolidating
tokenAddressandbalanceSlotinto a singletokenInfoparameter reduces coupling and prevents parameter mismatches.
51-56: Implementation correctly updated.The function logic properly uses
tokenInfo.HasSlotfor branching and accessestokenInfo.TokenAddressandtokenInfo.BalanceSlotconsistently.
3 participants
Summary by CodeRabbit